Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tax): add endpoints to manage tax rate rules #6557

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

srindom
Copy link
Collaborator

@srindom srindom commented Mar 1, 2024

What
Adds endpoints to manage tax rules on a tax rate:

  • Create a tax rule: POST /admin/tax-rates/:id/rules
  • Delete a tax rule: DELETE /admin/tax-rates/:id/rules/:rule_id
  • Replace tax rules: POST /admin/tax-rates/:id -- with { rules: [...] } in body.

Noteworthy things I bumped into

Updating nested relationships
A TaxRate can have multiple TaxRules and in this PR we enable users to replace all TaxRules associated with a TaxRate in one operation. If working with the module directly this can be done with:

taxModuleService.update(rateId, { rules: [{ ... }] })

Internally in the update function the TaxModule first soft deletes any TaxRules that exist on the TaxRate and then creates new TaxRules for the passed rules (see test).

A challenge arises when doing this in a compensatable way in a workflow. To see this imagine the following:

  1. updateTaxRatesWorkflow gets the current data for the tax rates to update. This includes the tax rates' rules.
  2. updateTaxRatesWorkflow calls taxModuleService.update with new rules.
  3. Internally, the tax module deletes the rules in 1. and creates new rules.
  4. Imagine an error happens in a following step and the workflow has to compensate.
  5. The workflow uses the data from 1. and calls upsert. The tax module may correctly update the previous tax rules so they are no longer soft deleted. However, upsert (at least not by default) doesn't delete the new rules that were created in 2.

As illustrated by 5. compensating the update is not pretty. To get around this I instead opted to let the workflow handle setting the rules for a rate that makes the compensation more straightforward to handle. See workflow here.

Using nested workflows
Initially, I wanted to use the setTaxRateRulesWorkflow within the updateTaxRatesWorkflow. And this worked great for the invoke phase. However, when I needed to compensate the update workflow (and hence also had to compensate the set rules workflow), I found that the workflow engine no longer had the set rules transaction in memory and therefore could not roll it back. (This is where I try to rollback, but the transaction id can't be found).

I therefore opted to copy the steps from the set tax rate rules workflow into the update tax rates workflow; however, once we figure out a good way to ensure we can compensate nested workflows we should move to the nested workflow instead.

This also made me realize that the current implementation of workflows that use refreshCartPromotions may create inconsistencies in case of failures (cc: @riqwan).

@srindom srindom requested review from a team as code owners March 1, 2024 09:52
Copy link

changeset-bot bot commented Mar 1, 2024

⚠️ No Changeset found

Latest commit: 2757527

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

changeset-bot bot commented Mar 1, 2024

⚠️ No Changeset found

Latest commit: 7cd9973

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Mar 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 4, 2024 10:05am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Mar 4, 2024 10:05am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Mar 4, 2024 10:05am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Mar 4, 2024 10:05am

Copy link
Contributor

@riqwan riqwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good, few comment and snake case to handle

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few comments

* fix

* fix

* fix: update tax rate with rule setting

* fix: add created_by
@kodiakhq kodiakhq bot merged commit 555eb41 into develop Mar 4, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants